Add test for precision conversion with `decl_hdf5` by Yushan-Wang · Pull Request #672 · pdidev/pdi

Yushan-Wang · 2026-04-14T11:04:46Z

List of things to check before making a PR

Before merging your code, please check the following:

Removed check for HDF5 version greater than 2.0.0 and related variable assignment.

…ushan-Wang/pdi into precision-conversion-with-hdf5

…into patch-hdf5-2.0.0

jbigot · 2026-04-15T09:41:58Z

Do you feel it would be a good candidate to demonstrate the use of #669 ?

…into patch-hdf5-2.0.0

Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>

jmorice91

look goods for me.
If your input double data is 1.5d0=1.50...0, how to know that the convention between HDF5 and gtest for transformation of double to int is the same?

Have you tested that?

As we use random number, it is a rare event that is arrived.

Yushan-Wang · 2026-04-20T09:38:02Z

look goods for me. If your input double data is 1.5d0=1.50...0, how to know that the convention between HDF5 and gtest for transformation of double to int is the same?

Have you tested that?

As we use random number, it is a rare event that is arrived.

HDF5 truncates floating-point numbers to integers upon precision conversion. So 1.5 would be output as 1. Same for static_cast

jmorice91 · 2026-05-07T14:45:41Z

+	ASSERT_GE(status, 0);
+
+#if defined(__clang__) && (__clang_major__ == 15)
+// Skipping the numerical comparaison because Clang 15 has known issues with ranges.


Perhaps do something like that in case we have issue with ranges,

std::array<std::array<int, N>, N> expected_int_array; for( int ii=0; ii<test_array.size(); ++ii) { std::transform(test_array[ii].cbegin(), test_array[ii].cend(), expected_int_array[ii].begin(), [](auto aa){ return static_cast<int>(aa); }); } // comparison EXPECT_EQ(expected_int_array, read_int_array);

JAuriac · 2026-05-07T14:30:09Z

+	hid_t dataset_id = H5Dopen2(file_id, "/double_ds", H5P_DEFAULT);
+	hid_t type_id = H5Dget_type(dataset_id);
+
+	EXPECT_TRUE(H5Tequal(type_id, H5T_IEEE_F64LE));


An error of value "-1" would pass this check ?

Suggested change

EXPECT_TRUE(H5Tequal(type_id, H5T_IEEE_F64LE));

EXPECT_GT(H5Tequal(type_id, H5T_IEEE_F64LE), 0);

JAuriac · 2026-05-07T14:32:11Z

+	dataset_id = H5Dopen2(file_id, "/float_ds", H5P_DEFAULT);
+	type_id = H5Dget_type(dataset_id);
+
+	EXPECT_TRUE(H5Tequal(type_id, H5T_IEEE_F32LE));


Similar to EXPECT_TRUE issue of H5T_IEEE_F64LE

JAuriac · 2026-05-07T14:32:27Z

+	dataset_id = H5Dopen2(file_id, "/int_ds", H5P_DEFAULT);
+	type_id = H5Dget_type(dataset_id);
+
+	EXPECT_TRUE(H5Tequal(type_id, H5T_STD_I32LE));


Similar to EXPECT_TRUE issue of H5T_IEEE_F64LE

JAuriac · 2026-05-07T14:34:31Z

+	hid_t file_id = H5Fopen("d2d_test.h5", H5F_ACC_RDONLY, H5P_DEFAULT);
+	hid_t dataset_id = H5Dopen2(file_id, "/double_ds", H5P_DEFAULT);
+	hid_t type_id = H5Dget_type(dataset_id);


May also check for read failure ?

Suggested change

hid_t file_id = H5Fopen("d2d_test.h5", H5F_ACC_RDONLY, H5P_DEFAULT);

hid_t dataset_id = H5Dopen2(file_id, "/double_ds", H5P_DEFAULT);

hid_t type_id = H5Dget_type(dataset_id);

hid_t file_id = H5Fopen("d2d_test.h5", H5F_ACC_RDONLY, H5P_DEFAULT);

hid_t dataset_id = H5Dopen2(file_id, "/double_ds", H5P_DEFAULT);

hid_t type_id = H5Dget_type(dataset_id);

ASSERT_GE(file_id, 0);

ASSERT_GE(dataset_id, 0);

ASSERT_GE(type_id, 0);

JAuriac · 2026-05-07T14:40:06Z

+	EXPECT_TRUE(H5Tequal(type_id, H5T_IEEE_F64LE));
+	std::array<std::array<double, N>, N> read_double_array;
+
+	herr_t status = H5Dread(dataset_id, H5T_NATIVE_DOUBLE, H5S_ALL, H5S_ALL, H5P_DEFAULT, read_double_array.data());


Pointer is not only a double, as it includes the static constexpr size_t N

Suggested change

herr_t status = H5Dread(dataset_id, H5T_NATIVE_DOUBLE, H5S_ALL, H5S_ALL, H5P_DEFAULT, read_double_array.data());

herr_t status = H5Dread(dataset_id, H5T_NATIVE_DOUBLE, H5S_ALL, H5S_ALL, H5P_DEFAULT, &read_double_array[0][0]);

JAuriac · 2026-05-07T14:40:56Z

+	EXPECT_FALSE(std::filesystem::exists("d2f_test.h5"));
+	EXPECT_FALSE(std::filesystem::exists("d2i_test.h5"));
+
+	static constexpr size_t const N = 100;


constexpr of a const

Suggested change

static constexpr size_t const N = 100;

static constexpr size_t N = 100;

JAuriac · 2026-05-07T14:45:28Z

+	EXPECT_FALSE(std::filesystem::exists("d2d_test.h5"));
+	EXPECT_FALSE(std::filesystem::exists("d2f_test.h5"));
+	EXPECT_FALSE(std::filesystem::exists("d2i_test.h5"));


Should we use std::filesystem::remove() before, to be sure ?

JAuriac · 2026-05-07T14:49:59Z

 endforeach(test_name)
+
+add_executable(decl_hdf5_conversion_test decl_hdf5_precision_conversion_test.cxx)
+target_link_libraries(decl_hdf5_conversion_test ${HDF5_DEPS} PDI::PDI_C GTest::gmock GTest::gmock_main GTest::gtest GTest::gtest_main)


Do GTest::gtest_main and GTest::gmock_main overlink ? Two other files of the repo also use this

jmorice91 · 2026-05-07T16:05:06Z

+	auto const test_array = make_a<std::array<std::array<double, N>, N>>();
+	PDI_expose("array", test_array.data(), PDI_OUT);
+
+	EXPECT_TRUE(std::filesystem::exists("d2d_test.h5"));


Perhaps, we should use assert her because if we don't have the file the comparison test
for double doesn't make any sense

Suggested change

EXPECT_TRUE(std::filesystem::exists("d2d_test.h5"));

ASSERT_TRUE(std::filesystem::exists("d2d_test.h5"));

Yushan-Wang added 10 commits March 20, 2026 14:31

add fix for hdf5 2.0.0

6c022f9

Fix MPI condition for HDF5 parallel support

4e88161

add test for precision conversion

b342921

Add 'd2s_precision' to ALL_TEST_NAMES

28e3989

Remove parallel HDF5 version check

3c39885

Fix HDF5 parallel check in FindNetCDF.cmake

dfa8769

Refactor HDF5 version checks in CMakeLists

94997ce

Removed check for HDF5 version greater than 2.0.0 and related variable assignment.

fix indent

b980a9d

Merge branch 'precision-conversion-with-hdf5' of https://github.com/Y…

bcdccf2

…ushan-Wang/pdi into precision-conversion-with-hdf5

update decl_hdf5/CHANGELOG.md

2774595

Yushan-Wang marked this pull request as ready for review April 14, 2026 11:53

Yushan-Wang requested review from benoitmartin88 and jbigot as code owners April 14, 2026 11:53

Yushan-Wang added 4 commits April 14, 2026 14:16

add cmake fix for version less than 4.3

9e51956

Add version checks for HDF5 in CMakeLists.txt

071dff9

Typo [skip ci]

6c5ceaa

Merge branch 'patch-hdf5-2.0.0' of https://github.com/Yushan-Wang/pdi …

1c5c227

…into patch-hdf5-2.0.0

Yushan-Wang force-pushed the precision-conversion-with-hdf5 branch 2 times, most recently from 059513f to 12dff81 Compare April 14, 2026 12:41

Yushan-Wang and others added 10 commits April 15, 2026 11:42

add fix for hdf5 2.0.0

2c2c58b

Fix MPI condition for HDF5 parallel support

ba0aed4

add cmake fix for version less than 4.3

5150802

Typo [skip ci]

1dcb3c2

Add version checks for HDF5 in CMakeLists.txt

b01bb1b

Remove redundant CMAKE_MODULE_PATH entry

bd034c8

Merge branch 'patch-hdf5-2.0.0' of https://github.com/Yushan-Wang/pdi …

3c5f4bc

…into patch-hdf5-2.0.0

remove 4.1 cmake patch

106bfbf

Update CMakeLists.txt

eeab5c1

Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>

Update plugins/decl_hdf5/CMakeLists.txt

6a33909

Co-authored-by: Julien Bigot <jbigot@users.noreply.github.com>

Yushan-Wang added 2 commits April 17, 2026 14:03

attempt for fixing CI faileur with spack oldest

d372511

indent

f60cb25

jbigot reviewed Apr 17, 2026

View reviewed changes

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx Outdated

jbigot reviewed Apr 17, 2026

View reviewed changes

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx Outdated

refactor: update HDF5 precision conversion test in a more compact way

7102952

jmorice91 reviewed Apr 20, 2026

View reviewed changes

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx

jbigot previously approved these changes Apr 20, 2026

View reviewed changes

jmorice91 reviewed Apr 20, 2026

View reviewed changes

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx Outdated

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx

skip precision conversion test when clang15

5dd30bb

Yushan-Wang dismissed jbigot’s stale review via 5dd30bb April 20, 2026 11:20

jbigot reviewed Apr 20, 2026

View reviewed changes

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx Outdated

Yushan-Wang added 3 commits April 20, 2026 16:37

avoid compiling precision conversion test with clang15

717cbc6

skip only numerical comparaison with clang15

4416895

empty line at eof

54ac0f0

jbigot force-pushed the main branch 6 times, most recently from c257c27 to ce93f25 Compare April 22, 2026 08:17

Yushan-Wang requested review from jbigot and jmorice91 April 23, 2026 08:22

Merge branch 'main' into precision-conversion-with-hdf5

3dc95ad

jbigot force-pushed the main branch from 41fed24 to b9ebda1 Compare April 24, 2026 09:13

iole-bolognesi reviewed Apr 29, 2026

View reviewed changes

Comment thread plugins/decl_hdf5/tests/compatibility_tests/decl_hdf5_precision_conversion_test.cxx

iole-bolognesi approved these changes May 6, 2026

View reviewed changes

jmorice91 reviewed May 7, 2026

View reviewed changes

JAuriac reviewed May 7, 2026

View reviewed changes

jmorice91 reviewed May 7, 2026

View reviewed changes

	EXPECT_TRUE(H5Tequal(type_id, H5T_IEEE_F64LE));
	EXPECT_GT(H5Tequal(type_id, H5T_IEEE_F64LE), 0);

	herr_t status = H5Dread(dataset_id, H5T_NATIVE_DOUBLE, H5S_ALL, H5S_ALL, H5P_DEFAULT, read_double_array.data());
	herr_t status = H5Dread(dataset_id, H5T_NATIVE_DOUBLE, H5S_ALL, H5S_ALL, H5P_DEFAULT, &read_double_array[0][0]);

	static constexpr size_t const N = 100;
	static constexpr size_t N = 100;

	EXPECT_TRUE(std::filesystem::exists("d2d_test.h5"));
	ASSERT_TRUE(std::filesystem::exists("d2d_test.h5"));

Conversation

Yushan-Wang commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List of things to check before making a PR

Uh oh!

jbigot commented Apr 15, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmorice91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Yushan-Wang commented Apr 20, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Yushan-Wang commented Apr 14, 2026 •

edited

Loading